Skip to content

CSE Machine fixes for variadic functions, closures & heap #1645

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 13 commits into from
Apr 9, 2024
Merged

CSE Machine fixes for variadic functions, closures & heap #1645

merged 13 commits into from
Apr 9, 2024

Conversation

@CZX123
Copy link
Contributor

@CZX123 CZX123 commented Apr 6, 2024 •
edited
Loading

Description

This PR fixes many issues related to the new heap addition.

Changes

  • Created duplicate Closure class for use in the CSE Machine
  • Made closureToJS use CSE Machine instead of the old interpreter
    • This will allow the built-in function apply_in_underlying_javascript to correctly display the environment and ensure objects get added to the heap correctly
  • Added new declaredName property in Closure, which stores the name of the constant variable the closure is assigned to
    • This is used for apply_in_underlying_javascript to create environment frames with better names
    • functionName will now always be in the form of x => ...
  • Correctly added the argument array of a variadic closure to the heap
  • Added environment property to functions returned from calling stream
  • Added move method in Heap for use in the frontend
  • Moved Stack and IStack to a separate file to avoid circular dependency issues

CZX123 added 3 commits April 6, 2024 19:33
@CZX123
CSE Machine fixes:
65c6712 
- make closureToJS use CSE machine instead of interpreter
- add variadic function array to heap
- handle functions from `stream`
- added heap move method
@CZX123
Formatting
93a30d1
@CZX123
Merge branch 'master' into cse-heap-updates
223e411
@CZX123 CZX123 self-assigned this Apr 6, 2024
@coveralls
Copy link

coveralls commented Apr 6, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 8608170634

Details

  • 90 of 95 (94.74%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 81.932%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/cse-machine/stack.ts 12 13 92.31%
src/cse-machine/closure.ts 46 50 92.0%
Totals Coverage Status
Change from base Build 8608095546: 0.07%
Covered Lines: 10687
Relevant Lines: 12693
💛 - Coveralls

@CZX123 CZX123 mentioned this pull request Apr 8, 2024
Merged
@CZX123 CZX123 marked this pull request as ready for review April 8, 2024 14:49
@CZX123 CZX123 mentioned this pull request Apr 8, 2024
Closed
1 task
martin-henz
martin-henz approved these changes Apr 8, 2024
View reviewed changes
Copy link
Member

@martin-henz martin-henz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work. Quite a few cleanups and improvements.

src/cse-machine/closure.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see "closure" in this folder instead of using the legacy interpreter implementation.

@martin-henz martin-henz merged commit dfce110 into master Apr 9, 2024
@martin-henz martin-henz deleted the cse-heap-updates branch April 9, 2024 00:08
@CZX123 CZX123 mentioned this pull request Apr 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martin-henz martin-henz martin-henz approved these changes

@RichDom2185 RichDom2185 Awaiting requested review from RichDom2185

Assignees

@CZX123 CZX123

Labels

None yet

Projects

No open projects
SA 2425 CP3108 P08: CSE machine animation
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@CZX123 @coveralls @martin-henz